-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OCPBUGS-4338: Apply all remediations associate with each ComplianceCheckResult #188
Conversation
@Vincent056: This pull request references Jira Issue OCPBUGS-4338, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/jira refresh |
@jhrozek: This pull request references Jira Issue OCPBUGS-4338, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/hold for QE, docs and PX |
pkg/controller/complianceremediation/complianceremediation_controller.go
Outdated
Show resolved
Hide resolved
pkg/controller/complianceremediation/complianceremediation_controller.go
Outdated
Show resolved
Hide resolved
pkg/controller/complianceremediation/complianceremediation_controller.go
Outdated
Show resolved
Hide resolved
pkg/controller/complianceremediation/complianceremediation_controller.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my manual testing went fine, just left a couple of minor comments in the PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
@Vincent056 It supposed to work all kubeleconfig rules. Not for rule ocp4-cis-kubelet-eviction-thresholds-set-soft-nodefs-inodesfree only. Ritht? Seems it is not working as expected:
|
In summary, it works for cr without -1/-2/-3/-4/-5 postfix; but not works for cr with -1/-2/-3/-4/-5 postfix. |
sorry, could you test with the new commit? It should be fixed now |
Repeated the test with 4.13.0-0.nightly-2022-12-19-122634 and new CO version from the PR. Now it working as expected. When applying a rule with/without "-1/-2/-3/-4/-5" postfix, the related crs will get applied. However, there is another problem, seems after rescan some cr in OutDated status, but I didn't see any difference from the current config and outdated config, details seen from: http://pastebin.test.redhat.com/1085853. Could you help to check? Thanks.
|
On Tue, Dec 20, 2022 at 06:40:34AM -0800, Xiaojie Yuan wrote:
Repeated the test with 4.13.0-0.nightly-2022-12-19-122634 and new CO version from the PR. Now it working as expected. When applying a rule with/without "-1/-2/-3/-4/-5" postfix, the related crs will get applied. However, there is another problem, seems after rescan some cr in OutDated status, but I didn't see any difference from the current config and outdated config, details seen from: http://pastebin.test.redhat.com/1085853. Could you help to check? Thanks.
btw Xiaojie showed me the problem earlier today and I think that it's an
unrelated bug because the objects in `.spec.current` and
`.spec.outdated` were the same.
I don't know if we should fix the issue in the same PR, just saying that
it's probably not related to the code changes in this PR, but rather
some old bug that got triggered.
|
Let me look into this |
Looks like this PR needs to be rebased. |
On Tue, Dec 20, 2022 at 06:38:46PM -0800, Vincent Shen wrote:
> On Tue, Dec 20, 2022 at 06:40:34AM -0800, Xiaojie Yuan wrote: Repeated the test with 4.13.0-0.nightly-2022-12-19-122634 and new CO version from the PR. Now it working as expected. When applying a rule with/without "-1/-2/-3/-4/-5" postfix, the related crs will get applied. However, there is another problem, seems after rescan some cr in OutDated status, but I didn't see any difference from the current config and outdated config, details seen from: http://pastebin.test.redhat.com/1085853. Could you help to check? Thanks.
> btw Xiaojie showed me the problem earlier today and I think that it's an unrelated bug because the objects in `.spec.current` and `.spec.outdated` were the same. I don't know if we should fix the issue in the same PR, just saying that it's probably not related to the code changes in this PR, but rather some old bug that got triggered.
Let me look into this
btw I don't think the issue is related and therefore should be a
separate PR with a separate ticket.
|
Thanks, I will file in a separate ticket for it |
/test e2e-aws |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's a small bug we need to fix
pkg/controller/complianceremediation/complianceremediation_controller.go
Show resolved
Hide resolved
Make Compliance Operator to apply all the related remediations for one CCR
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhrozek, Vincent056 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@xiaojiey PTAL for formal QE-ack |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - still waiting on a cluster to test this.
Hi Vincent, what is the option to still go one by one applying the remediations, if the customer wants to do so or for debugging purposes? |
they shouldn't apply remediations associated with a rule one by one, they should be applied at once. It might break their cluster if remediation was partially applied associated with a rule |
LGTM. |
Adding qe-approved since @xiaojiey tested the PR and the issue she saw was agreed to be a separate problem. |
@Vincent056: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-4338 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Make Compliance Operator apply all the related remediations for one ComplianceCheckResult at once, this helps users who use manual remediation, this feature will look for all the related remediations for a ComplianceCheckResult when one remediation is applied.
For ex. we have
remediations, when a user applies either one of them, we will apply all the other remediations associated with the rule.
[OCPBUGS-4338]https://issues.redhat.com/browse/OCPBUGS-4338